Conversation
| from awscrt.io import ClientBootstrap, ClientTlsContext, SocketOptions | ||
| from dataclasses import dataclass | ||
| from awscrt.mqtt5 import Client as Mqtt5Client | ||
| from awscrt.mqtt5 import Client as Mqtt5Client, SdkMetrics |
There was a problem hiding this comment.
Instead of pulling SdkMetrics in from mqtt5, should we instead implement an mqtt3 version of SdkMetrics? It appears there's precedence for this with the double implementation of OperationStatisticsData. Unsure if this is a good or bad practice but it's one we've previously used. This could also potentially allow us to set different things by default for mqtt3 vs. mqtt5 or it could add confusion and a second place to update things when we make changes...
There was a problem hiding this comment.
I'd prefer not to duplicate the structure across MQTT3 and MQTT5. Maintaining it in two places increases the risk of them going out of sync over time. I realize that's the pattern we used in Python, but I wouldn't consider it a good practice to follow.
If we ever need different defaults for MQTT3 vs. MQTT5, we can always inherit from the base structure at that point.
There was a problem hiding this comment.
We could add a separate module, like we did with req-resp client. With meta data implemented, the metrics functionality should be big enough to be in its own module.
sbSteveK
left a comment
sbSteveK
left a comment
There was a problem hiding this comment.
Seems solid. It looks like MQTT3 and MQTT5 will have to go through different binding paths when metrics is expanded. Would be ideal to use the same for both but unsure if that's possible without changes underneath?
Unfortunately, I don't think we can change the binding path for MQTT3 and MQTT5 without changes underneath... |
3 participants
Issue #, if available:
Description of changes:
IoTDeviceSDKMetricsand metrics related features , CRT is now appending AWS metrics by default.enableMetricsoption to allow user disable metrics.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.